Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require users of msg_send! to specify the return type #83

Closed

Conversation

SimonSapin
Copy link

This is a breaking change, to prevent misuses such as #62.

This is a breaking change, to prevent misuses such as
SSheldon#62
@SimonSapin
Copy link
Author

I found after making this patch #62 (comment) which suggests an alternative idea: requiring the return type to implement some (unsafe) trait that is not implemented for !. This has the advantage of not changing every single call sites in users of the crate.

@SSheldon
Copy link
Owner

@SimonSapin you mention in rust-lang/rust#65355 (comment):

The match expression where an unconstrained type parameter is unified with the type of a panic!() expression is hidden in a macro expansion

If we remove the match and panic expressions, will that help? If so, I can explore some refactor there. Would a trivial reordering like this prevent the types from being unified?

let r = $crate::__send_message(...);
if r.is_err() {
    panic!("{}", r.as_ref().unwrap_err());
}
return r.unwrap();

@SimonSapin
Copy link
Author

I don’t know, it’s worth trying

@SimonSapin
Copy link
Author

Is there a reason the panic is part of a macro expansion rather than in __send_message or some other library function?

@SSheldon
Copy link
Owner

Just tested that change on rustc 1.35 and confirmed that with it, I get a compile error at the usage site of the macro:

error[E0282]: type annotations needed
  --> examples/example.rs:28:9
   |
28 |         msg_send![*obj, retain];
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
   |         |
   |         cannot infer type for `R`
   |         consider giving `r` a type
   |
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

Neat! Didn't realize that I could get the misuse case to compile error. I think we should just make a change like that.

@SSheldon
Copy link
Owner

@SimonSapin I wanted the panic to report the line and file where msg_send! was used rather than always reporting a line from inside the objc crate, especially for when people hit errors and don't have backtraces enabled. That was all!

@SimonSapin
Copy link
Author

Re line numbers I see, that makes sense.

It’s rather extreme, but how would you feel about yanking all current versions? See rust-lang/rust#65355 (comment)

@SSheldon
Copy link
Owner

Regarding yanking, I wouldn't love that, but we can keep it under consideration. First we'll have to figure out how the heck to get everyone to fix the existing misuses of msg_send!...

Closing this in favor of df957f2, a more backwards compatible way to prevent this.

@SSheldon SSheldon closed this Oct 16, 2019
@cmyr
Copy link

cmyr commented Oct 22, 2019

Just noticed that this was published as 0.2.7, e.g. a patch; noticed because ci started failing in a downstream crate. Shouldn't this at least be 0.3?

Edit okay, I found #62, ignore me ✌️

@SSheldon
Copy link
Owner

@cmyr sorry for the inconvenience! A bit more on my reasoning in rust-lang/rust#65355 (comment) and #86 (comment).

If I had published the change as 0.3, you would have undefined behavior when ! stabilizes. Using msg_send! without a return type was never an intentional feature of this crate, and with the resulting undefined behavior, I considered this a soundness fix and an unfortunate but necessary breakage.

Let me know if I can help the crate you're using get fixed! We tested the biggest users of objc and made sure they had fixes ready, but the crate you're using may have slipped under the radar.

@cmyr
Copy link

cmyr commented Oct 22, 2019

@SSheldon no worries, the rationale makes sense, the fix wasn't that painful. I'm glad I can do let () = .. instad of let _: () = ..,. In any case this is up as druid#219. In that thread raph makes a good point, which is that reflexively casting unused results to () might cause other problems. Is there any room for concern here? I would expect to be able to cast any id to a void*, but I'm not sure about casting to void. 😕

edit: here's an example where I've casted an NSTimer* to ().

edit edit: also I assume you've seen https://www.mikeash.com/pyblog/objc_msgsends-new-prototype.html? I found it really interesting. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants